feat(minidump): Guard against compressed minidump streams#5984
feat(minidump): Guard against compressed minidump streams#5984tobias-wilfert wants to merge 11 commits into
Conversation
| project_id = 42 | ||
| minidump_content = b"MDMP content" | ||
| log_content = b"Some log file content" | ||
| log_content = b"\x1f\x8b Some log file content" |
There was a problem hiding this comment.
Add this to ensure that compressed attachment still work.
| Err(_) => { | ||
| return Err(item.reject_err(Outcome::Invalid(DiscardReason::InvalidMinidump))); | ||
| } |
There was a problem hiding this comment.
We also reject the item here if we just fail to read for the stream for any reason, but I think that is ok.
| } | ||
| } | ||
| let head = head.freeze(); | ||
| let replay = stream::once(future::ready(Ok(head.clone()))).chain(stream); |
| if head.starts_with(GZIP_MAGIC_HEADER) | ||
| || head.starts_with(XZ_MAGIC_HEADER) | ||
| || head.starts_with(BZIP2_MAGIC_HEADER) | ||
| || head.starts_with(ZSTD_MAGIC_HEADER) |
There was a problem hiding this comment.
nit: You could put this in a helper function get_compression(data: &[u8]) -> Option<HttpEncoding> and share the helper with decoder_from.
There was a problem hiding this comment.
Not sure we can use it in decoder_from since that logic is slightly more sophisticated:
relay/relay-server/src/endpoints/minidump.rs
Lines 143 to 162 in 0f95e1e
There was a problem hiding this comment.
My thinking was something like
fn decoder_from(minidump_data: Bytes) -> Option<Box<dyn Read>> {
match get_compression(minidump_data) {
HttpEncoding::Gzip => Some(Box::new(GzDecoder::new(Cursor::new(minidump_data))))There was a problem hiding this comment.
Discussed offline, we're hesitant to add uncommon variants to the HttpEncoding enum so this would require a separate enum -> not worth the effort.
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if head.starts_with(GZIP_MAGIC_HEADER) | ||
| || head.starts_with(XZ_MAGIC_HEADER) | ||
| || head.starts_with(BZIP2_MAGIC_HEADER) | ||
| || head.starts_with(ZSTD_MAGIC_HEADER) |
There was a problem hiding this comment.
Discussed offline, we're hesitant to add uncommon variants to the HttpEncoding enum so this would require a separate enum -> not worth the effort.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
…/no-compressed-streams
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 456f391. Configure here.
| Ok(stream) => stream, | ||
| Err(_) => { | ||
| return Err(item.reject_err(Outcome::Invalid(DiscardReason::InvalidMinidump))); | ||
| } |
There was a problem hiding this comment.
Multipart field body not drained
Medium Severity
When a streamed minidump is rejected as compressed, only the peeked prefix is read and the rest of the multipart Field stream is dropped without being consumed. Later parts in the same upload may not parse correctly, so typical multi-file minidump requests can fail unpredictably after rejection.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 456f391. Configure here.
2 participants
Adds a check to ensure we are not streaming compressed minidumps to objectstore (since neither it not symbolicator can currently handle them).
FIX INGEST-891